Skip to content

Implement summonIgnoring #22417

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Feb 18, 2025
Merged

Implement summonIgnoring #22417

merged 4 commits into from
Feb 18, 2025

Conversation

jchyb
Copy link
Contributor

@jchyb jchyb commented Jan 20, 2025

Related discussion: #21909
We have yet to confirm/discuss whether this should/can be merged, but otherwise I consider this a complete implementation.
One thing to note - I decided it would be better for the ignored list to not affect indirect implicit searches (like in the tests/run-macros/summonIgnoring-nonrecursive test.

@soronpo
Copy link
Contributor

soronpo commented Jan 21, 2025

If we do this, I think this functionality should be also in compiletime.summonIgnoring

@jchyb
Copy link
Contributor Author

jchyb commented Jan 21, 2025

It's actually macros-only for now, but of course users could just prepare their own macro method that summons what they want and returns it for use elsewhere

@Gedochao Gedochao added the needs-minor-release This PR cannot be merged until the next minor release label Jan 22, 2025
@Gedochao Gedochao requested review from odersky and noti0na1 January 22, 2025 15:27
@jchyb jchyb changed the title Draft: Implement summonIgnoring Implement summonIgnoring Jan 22, 2025
@jchyb jchyb marked this pull request as ready for review January 24, 2025 09:43
@@ -928,8 +928,8 @@ trait Implicits:
/** Find an implicit argument for parameter `formal`.
* Return a failure as a SearchFailureType in the type of the returned tree.
*/
def inferImplicitArg(formal: Type, span: Span)(using Context): Tree =
inferImplicit(formal, EmptyTree, span) match
def inferImplicitArg(formal: Type, span: Span, ignored: Set[Symbol])(using Context): Tree =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think in this case it's overall simpler to use a default = Set.empty for ignored.

@@ -1679,6 +1679,8 @@ trait Implicits:
else ctxImplicits.eligible(wildProto)
else implicitScope(wildProto).eligible

val preEligible =
prePreEligible.filter(candidate => !ignored.contains(candidate.implicitRef.underlyingRef.symbol))
Copy link
Contributor

@odersky odersky Feb 17, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will copy each list returned by eligible, so it's a significant memory churn to cater for an edge case. An improvement would use filterConserve, but I think it's even better to add a conditional. Something like that:

var preEligible = ... // rhs of prePreEligible
if !ignored.isEmpty then 
  preEligible = preEligible.filter(...)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, good point. Thank you!

@@ -1082,7 +1082,7 @@ trait Implicits:
* it should be applied, EmptyTree otherwise.
* @param span The position where errors should be reported.
*/
def inferImplicit(pt: Type, argument: Tree, span: Span)(using Context): SearchResult = ctx.profiler.onImplicitSearch(pt):
def inferImplicit(pt: Type, argument: Tree, span: Span, ignored: Set[Symbol])(using Context): SearchResult = ctx.profiler.onImplicitSearch(pt):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also use a default argument here.

@odersky odersky removed their assignment Feb 17, 2025
@jchyb jchyb requested a review from odersky February 18, 2025 11:25
@jchyb jchyb assigned odersky and unassigned jchyb Feb 18, 2025
Copy link
Contributor

@odersky odersky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reading the comments again, I think @soronpo has a good point. Should summonIgnoring also be in compiletime. I think it would make sense. @jchyb WDYT?

@odersky odersky assigned jchyb and unassigned odersky Feb 18, 2025
@jchyb
Copy link
Contributor Author

jchyb commented Feb 18, 2025

I was a little concerned about how the API would look for that since we do not have access to quotes.reflect.Symbol outside of macros - probably the best solution would be something like def summonIgnoring[T](inline ignored: Any*), with some kind of symbol extraction from the passed terms, which could be tricky. But to be honest, it's not the first time I was asked about it (the chimney maintainer also mentioned it), so I'll try to prepare something now

@odersky
Copy link
Contributor

odersky commented Feb 18, 2025

@jchyb I see. I had overlooked that point. That does not seem to have an obvious solution. So I propose we defer this question and merge this PR as is.

@jchyb jchyb merged commit da6c61c into scala:main Feb 18, 2025
29 checks passed
@jchyb jchyb deleted the summonIgnore branch February 18, 2025 14:19
@tgodzik tgodzik added the release-notes Should be mentioned in the release notes label Feb 24, 2025
@WojciechMazur WojciechMazur added this to the 3.7.0 milestone Mar 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs-minor-release This PR cannot be merged until the next minor release release-notes Should be mentioned in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants